Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WINTERMUTE: Add keymappings #2051

Merged
merged 5 commits into from Feb 19, 2020
Merged

Conversation

lolbot-iichan
Copy link
Contributor

No description provided.

@lolbot-iichan lolbot-iichan changed the title WINTERMUTE: Support keymapping (WIP) WINTERMUTE: Add keymapping (WIP) Feb 9, 2020
@lolbot-iichan lolbot-iichan changed the title WINTERMUTE: Add keymapping (WIP) WINTERMUTE: Add keymappings (WIP) Feb 9, 2020

Common::KeymapArray result = Keymap::arrayOf(engineKeyMap);

Common::String targetString = target;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The target is user-controlled. It's possible to edit it when adding a game. Is fetching the gameid from the configuration manager not working for you? Common::String gameId = ConfMan.get("gameid", target);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Never noticed "gameid" property before.

@lolbot-iichan
Copy link
Contributor Author

lolbot-iichan commented Feb 13, 2020

Current state of this PR:

  1. Standard in-game controls & game-specific controls for all 90+ games listed at https://wiki.scummvm.org/index.php?title=Wintermute/Controls are all added to detection.cpp, except for "onehelluvaday" demo, which features an action binded to Shift keypress. It seems that currently it's impossible to either add default input mapping for Shift, or assign it manually with GUI. Original inputs are all marked with "//original [keyboard|mouse|joy]" comments.

  2. Text input engine-hardcoded controls are still hardcoded, I don't plan to change it in this PR.

  3. Extra keyboard input mappings (UP+DOWN or PGUP+PGDN) are added for a couple of games. They are marked with "// extra keyboard". Ideas begind assigning extra keyboard keys:

  • original keymaps are never changed, if both UP+DOWN and PGUP+PGDN is already used, then nothing is added.
  • if game is using mouse wheel and does not use UP+DOWN or PGUP+PGDN pair -> map one of them to the mouse wheel
  1. Extra joystick mappings are added for all games. They are marked with "// extra joy". Ideas behind assigning JOY buttons:
  • All games are using LMB=JOY_A, RMB=JOY_B, ESC=JOY_X.
  • Game's most useful action (usually, HINTS or INV) is binded to JOY_Y.
  • If game has paired controls like scrolling, changing pages, walking, switching between 2 GUI types -> assign those to JOY arrows.
  • A few games features unpaired actions binded to JOY_LEFT and/or JOY_RIGHT (like, Face Noir's INV on JOY_LEFT and HINTS on JOY_Y). I'm not sure if it's ok or not to call commands with DPAD keys. Maybe JOY_R+Y would be nice to have, maybe not - not use...
  • Enter is curretly unmapped to JOY. It would be really nice if JOY_R+JOY_X would be allowed for Enter. Almost all of listed games are using Enter at windows like "Are you sure you want %s", but I'm not sure if it's a shortcut for OK button in all the games or some games are not fully playable without ability to press Enter. I'd better have a JOY mapped combo for it, just in case some game absolutely require it.
  • All cheats are unmapped to JOY. It would be really nice if JOY_R+JOY_B combo would be allowed for cheats & secrets. FoxTail has 2 cheats, for JOY_R+JOY_A would be useful as well.
  1. Extra mouse input mappings (MOUSE_WHEELUP+MOUSE_WHEELDOWN, MOUSE_MIDDLE) are added. They are marked with "// extra mouse". Ideas behind assigning extra MOUSE buttons:
  • original keymaps are never changed, if MOUSE_MIDDLE and/or MOUSE_WHEELUP+MOUSE_WHEELDOWN are already used, rules below does not apply.
  • If MOUSE_RIGHT is not actually used, it may be remapped (used only for "Wilma Tetris", which does not use RightClick event).
  • JOY_Y has some action -> assign MOUSE_ MIDDLE to the same action (except for Face Noir, where INV action is mapped to MOUSE_MIDDLE in original keymap, while I chose HINTS to be assigned to JOY_Y).
  • If game has paired controls like scrolling, changing pages, switching between 2 GUI types -> assign those action assigned to MOUSE_WHEELUP/MOUSE_WHEELDOWN arrows

@lolbot-iichan lolbot-iichan changed the title WINTERMUTE: Add keymappings (WIP) WINTERMUTE: Add keymappings Feb 13, 2020
@bgK
Copy link
Member

bgK commented Feb 13, 2020

Humongous work! We can merge this whenever you feel it's ready.

It seems that currently it's impossible to either add default input mapping for Shift, or assign it manually with GUI.

I'll look into fixing that, there is no reason not to be able to remap the modifier keys.

@lolbot-iichan
Copy link
Contributor Author

Humongous work! We can merge this whenever you feel it's ready.

Thanks! I guess I'll wait for #2052 to complete and will do a rebase, since my branch conflicts with that PR.

Meanwhile, current open questions:

  • Should I add JOY_R+JOY_X for Enter or leave it unmapped?
  • Should I add JOY_R+JOY_B for secrets or leave them unmapped?
    If at least one of those is yes, then what is the syntax? act->addDefaultInputMapping("JOY_R+JOY_X") does not seem to work for me (GUI does not list this combo).

@bgK
Copy link
Member

bgK commented Feb 14, 2020

Should I add JOY_R+JOY_X for Enter or leave it unmapped?
Should I add JOY_R+JOY_B for secrets or leave them unmapped?

At the moment, joystick button combos are not expressible using the keymapper. I suggest leaving those actions unmapped with a comment describing the ideal bindings. Users can map them to device specific buttons (triggers, stick press, right stick axes) if needed.

@bgK
Copy link
Member

bgK commented Feb 14, 2020

It seems that currently it's impossible to either add default input mapping for Shift, or assign it manually with GUI.

I'll look into fixing that, there is no reason not to be able to remap the modifier keys.

Done in 29dd15a.

act = new Action("RETURN", _("Confirm"));
act->setKeyEvent(KeyState(KEYCODE_RETURN, ASCII_RETURN));
act->addDefaultInputMapping("RETURN"); // original keyboard
//TODO: extra joy control, e.g. "JOY_R+JOY_X"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, indent all of these TODO comments properly (more follow below, no reason to repeat this comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Intended in line with code with tabs.

@bluegr
Copy link
Member

bluegr commented Feb 16, 2020

Extraordinary work! Well done!

Could you please fix the indentation of the TODO comments? Other than that, it's an amazing effort, kudos!

@lolbot-iichan
Copy link
Contributor Author

lolbot-iichan commented Feb 16, 2020

Rebased, removed debugger handling (it's already done at #2052), added Shift hotkey for One Helluva Day demo, moved TODOs to the right.

Done in 29dd15a.

Thanks, supported that at 9e267aa.

Could you please fix the indentation of the TODO comments?

Fixed at 9e267aa.

We can merge this whenever you feel it's ready.

I feel it's ready.

gameId == "rhiannon" ||
gameId == "shinestar"
) {
act = new Action("SKIP", _("Skip"));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
act = new Action("SKIP", _("Skip"));
act = new Action(kStandardActionSkip, _("Skip"));

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, fixed! Maybe we should also introduce more Standard Actions for things like "HINT", "WTF", "ACTNXT", "PAGEDN", "SCRLUP", etc?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, feel free to add some. I'm not convinced standard actions are very useful at the moment. We set the default bindings for mouse+keyboard and gamepad when we define the actions. The backend changing the defaults for the standard actions is very likely to result in nonsensical / conflicting keymaps.

One possible future change is the evolution of the virtual keyboard into some sort of more general touch oriented on-screen input method. Having standard actions in that context would be nice as we could bundle a set of corresponding icons.

engines/wintermute/detection.cpp Outdated Show resolved Hide resolved
engines/wintermute/detection.cpp Outdated Show resolved Hide resolved
engines/wintermute/detection.cpp Outdated Show resolved Hide resolved
engines/wintermute/detection.cpp Outdated Show resolved Hide resolved
engines/wintermute/detection.cpp Outdated Show resolved Hide resolved
//TODO: extra joy control, e.g. "JOY_R+JOY_B"
gameKeyMap->addAction(act);

act = new Action("DBG", _("Debug print"));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has the same ID as the action for opening the debugger. I suggest renaming it to something else.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed to "DBGTXT", thanks for pointing this.
@bluegr @ccawley2011 @bgK Should we introduce some convention to avoid things like this in the future? Like, "non-standard actions IDs should always be 6+ characters long"?

Copy link
Member

@bgK bgK Feb 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well the action ids are namespaced by the keymap names. There was no real conflict here. The only issue is if a backend sets a default for a standard action that happens to have the same name as game action. I'm not sure it's worth the effort of enforcing a naming convention just for that.

engines/wintermute/detection.cpp Show resolved Hide resolved
engines/wintermute/detection.cpp Outdated Show resolved Hide resolved
@lolbot-iichan
Copy link
Contributor Author

Fixed errors from above + splited "5ma||5ld||dirtysplit||projectjoe" keymap into two, since they slightly different original controls + fixed action ID collision at Alpha Polaris.

@bluegr
Copy link
Member

bluegr commented Feb 19, 2020

Great work, once again! Thanks for fixing the pending issues. Merging

@bluegr bluegr merged commit 887db8a into scummvm:master Feb 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants